Simplify Link annotation handling #8972

RX14 · 2020-03-29T17:57:03Z

This pull request deprecates the @[Link("lib", static: true)] request to prefer static linking of libraries. The only use of this is a @[Link("gc", static: true)] in the crystal repo. I have been unable to find any exmaples of static: true being used outside of this repo. The reason for this, I suspect, is because it doesn't work. People who want the existing behaviour can simply pass ldflags: "-l :libgc.a"

Also deprecated is passing positional arguments to @[Link] other than "lib" (nobody uses that either).

Because of these simplifications, we can implement a new, simpler, linking algorithm with less code.

First, a pkg_config link argument is added, since almost all libraries have a different pkg-config name to the lib<foo>.so name. For example, libz.so is zlib.pc, libgc.so is bdw-gc.pc. Only one of 10 different libs used by the crystal standard library was correctly detected using the original mechanism. Allowing pkg-config module names to be specified correctly allows the pkg-config mechanism to work properly, which makes linking far more reliable. The fallback remains -lfoo as before.

When neither static: true or --static is specified, the current algorithm is identical to before, but with the pkg_config argument added. static: true is now ignored, and passing --static simply passes -static to the linker and --static to pkg-config.

straight-shoota · 2020-03-29T18:26:52Z

src/annotations.cr

@@ -27,7 +27,7 @@ end
 annotation Flags
 end

-# A `lib` can be marked with `@[Link(lib : String, ldflags : String, static : Bool, framework : String)]`
+# A `lib` can be marked with `@[Link(lib : String, *, ldflags : String, static : Bool, framework : String, pkg_config : String)]`


You should add that static is deprecated. Not in this line but further down. And maybe not mention it here at all.

Should also note that CRYSTAL_LIBRARY_PATH is added to the lookup paths.

I tweaked the link annotation docs. CRYSTAL_LIBRARY_PATH was already mentioned further down, but I removed some old references to hardcoded paths.

I removed mention of static entirely, I presume that'll be gone by 1.0 if all goes well.

src/compiler/crystal/codegen/link.cr

bcardiff · 2020-04-02T20:09:06Z

src/compiler/crystal/codegen/link.cr

-          flags << " -L#{path}"
+        # First, check pkg-config for the pkg-config module name if provided, then
+        # check pkg-config with the lib name, then fall back to -lname
+        if (pkg_config_name = ann.pkg_config) && (flag = pkg_config(pkg_config_name, static_build))


This is giving more precedence to the pkg-config settings for the lib, instead of giving priority to the CRYSTAL_LIBRARY_PATH as it should. Otherwise having a local package for bdw-gc will override the patched embedded version we ship.

yeah, it will, I don't think CRYSTAL_LIBRARY_PATH was a good idea because it binds you to the "path" model of library lookup, which is outdated and replaced with pkgconfig (and PKG_CONFIG_PATH)

A better solution is CRYSTAL_LIBRARY_OVERRIDES="gc=/path/to/whatever otherlib=-lwhatever" which is far more robust and flexible by allowing you to replace any link annotation with arbitrary ldflags.

waj · 2020-05-20T16:32:23Z

@RX14 I like the overall idea of this PR. The Link annotation needed some love for long time.

However, I'm not so sure yet about deprecating the static option. This was introduced long time ago (7ec3a82) to solve a specific issue. I'm not proud at all of that code and I'm open to suggestions for alternatives, but this workaround was used to force static linking when both static and dynamic libraries are available, specially when trying to use a patched version of the GC for example, while the system wide library is still available. Also, the -l:libgc.a option doesn't work everywhere (it doesn't on macOS). I'll be really happy actually if that library lookup is removed from the source, as I understand that should be the task of the linker only, but let's make sure all scenarios are still supported.

I love the idea of CRYSTAL_LIBRARY_OVERRIDES and I think we should implement it soon (probably after 1.0 though). Now, to make it work it needs a name to use as reference. The lib argument is now optional, so I'm thinking we should make it mandatory (and maybe rename it to just name?). What do you think?

asterite · 2020-05-20T16:41:46Z

CRYSTAL_LIBRARY_OVERRIDES can work by specifying the name of a lib. For example lib LibPCRE. Then you would pass CRYSTAL_LIBRARY_OVERRIDES="LibPCRE=...". There's really no need for the "name" to be what you specify to override. Specially, with the approach proposed here we have to check whether "name" was already specified somewhere else, but there's no way to define two "LibPCRE", there's always just one or none.

By the way, I mentioned doing it this way more than a year ago: https://forum.crystal-lang.org/t/rfc-link-configuration/546/2?u=asterite . And there's no need for an environment variable to do it, it can just be a compiler flag.

waj · 2020-05-20T16:46:21Z

@asterite There are some lib that has more than one Link annotation. Maybe overriding all of them at the same time it's what we want? I don't know.

jhass · 2020-05-20T17:05:16Z

Yeah, the lib/link relationship is many to many, additionally to one lib definition having many link annotations, the same link annotation could also be on many lib definitions. Using the lib name the behavior seems unclear in either scenario.

RX14 · 2020-05-20T19:35:06Z

Yeah, I was thinking of making Link's name mandatory.

I appreciate that static: true can be useful sometimes, but I've only found a single usage, so I'd like to try removing it and see if we get any complaints. Without any details of the specific issue, we're just keeping complicated, hard to justify code around for undetermined reasons.

waj · 2020-05-20T20:31:09Z

Oh, but I thought I made the issue clear. For example, if there is a system wide installed libgc.so, are you sure the libgc.a shipped with Crystal will still be picked? IIRC some linkers might prefer the shared object instead of the static library unless --static is specified.

RX14 · 2020-05-22T10:47:55Z

Presumably in the distribution, where we would ship a libgc.a, we'd use CRYSTAL_LIBRARY_OVERRIDES, because we want to override the linker's "common sense" that the system libraries are good enough. But that needs to be implemented first.

In time, when libgc.so on these distributions is universally up-to-date with the latest API, we can let the linker have it's common sense back :P

bcardiff · 2020-05-26T15:41:20Z

@RX14 can you rebase this on master due to conflicts?

Static linking per-library is not supported well on any platform we support, and the feature is rarely used. Deprecate it so we can simplify the linking process.

RX14 · 2020-05-26T19:22:25Z

Yep.

bcardiff

The comments in https://github.com/crystal-lang/crystal/pull/8972/files#r399834746 haven't been addressed.

We can merge this PR and see how it goes. Maybe in the future the compiler will need to add some pkg-config path to avoid breaking in some user setup. Like having a gc.pc that will prevent the embedded libraries to be linked. That will offer some workaround to the lookup priority changed.

The following entries in crystal-book should be updated

https://github.com/crystal-lang/crystal-book/blob/master/using_the_compiler/README.md (due to CRYSTAL_LIBRARY_PATH mentions)
https://github.com/crystal-lang/crystal-book/blob/master/syntax_and_semantics/c_bindings/lib.md (due to Link annotation description)

RX14 · 2020-05-26T20:38:39Z

I'd like to implement CRYSTAL_LIBRARY_OVERRIDES sooner rather than later, and definitely start requiring a name for every link annotation, but yeah we should see how it goes.

I'll address those comments.

bcardiff · 2020-05-26T20:55:46Z

I would avoid designing CRYSTAL_LIBRARY_OVERRIDES before we discuss the story of a compiler configuration files in the .crystal folder. That will probably cover many stories together.

How to override the libs link annotation is definitely one of the main stories to cover there.

RX14 · 2020-05-26T21:01:21Z

I would avoid designing CRYSTAL_LIBRARY_OVERRIDES before we discuss the story of a compiler configuration files in the .crystal folder. That will probably cover many stories together.

I think there's definitely need for a system/user-level override as well as project-level, and an env var is a good way to do that. Perhaps you want to wait on that but I'm not convinced it's neccesary.

src/annotations.cr

Co-authored-by: Brian J. Cardiff <bcardiff@gmail.com>

straight-shoota reviewed Mar 29, 2020

View reviewed changes

bcardiff reviewed Apr 2, 2020

View reviewed changes

RX14 mentioned this pull request Apr 16, 2020

cl.exe c:crystalllvm-10.0.0.srcReleaseinllvm-config.exe --libs --system-libs --ldflags 2> /dev/null #9089

Closed

bcardiff added this to the 0.35.0 milestone May 26, 2020

bcardiff added the topic:compiler label May 26, 2020

RX14 added 3 commits May 26, 2020 20:12

Deprecate per-library static linking hints

272041b

Static linking per-library is not supported well on any platform we support, and the feature is rarely used. Deprecate it so we can simplify the linking process.

Deprecate positional arguments in Link annotations

a5bab32

Simplify Link annotation handling

eec1cdf

RX14 force-pushed the feature/link-fix branch from 733af69 to eec1cdf Compare May 26, 2020 19:20

bcardiff approved these changes May 26, 2020

View reviewed changes

RX14 mentioned this pull request May 26, 2020

Port to NetBSD #9360

Merged

Tweak Link annotation docs

5e19c67

bcardiff approved these changes May 26, 2020

View reviewed changes

src/annotations.cr Outdated Show resolved Hide resolved

Update src/annotations.cr

4ff7da6

Co-authored-by: Brian J. Cardiff <bcardiff@gmail.com>

straight-shoota approved these changes May 26, 2020

View reviewed changes

bcardiff merged commit 499d08d into crystal-lang:master May 26, 2020

bcardiff added the breaking-change label May 27, 2020

jhass mentioned this pull request Jun 1, 2020

Specify pkgconfig name for libevent #9395

Merged

waj mentioned this pull request Jul 1, 2020

error: linker OpenSSL::BIO::CRYSTAL #9477

Closed

bcardiff mentioned this pull request Jul 28, 2020

Update llvm-config lookup crystal-lang/clang.cr#8

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify Link annotation handling #8972

Simplify Link annotation handling #8972

RX14 commented Mar 29, 2020

straight-shoota Mar 29, 2020

straight-shoota Mar 29, 2020

RX14 May 26, 2020

bcardiff Apr 2, 2020

RX14 Apr 2, 2020 •

edited

waj commented May 20, 2020

asterite commented May 20, 2020

waj commented May 20, 2020

jhass commented May 20, 2020

RX14 commented May 20, 2020 •

edited

waj commented May 20, 2020

RX14 commented May 22, 2020 •

edited

bcardiff commented May 26, 2020

RX14 commented May 26, 2020

bcardiff left a comment

RX14 commented May 26, 2020

bcardiff commented May 26, 2020

RX14 commented May 26, 2020 •

edited

Simplify Link annotation handling #8972

Simplify Link annotation handling #8972

Conversation

RX14 commented Mar 29, 2020

straight-shoota Mar 29, 2020

Choose a reason for hiding this comment

straight-shoota Mar 29, 2020

Choose a reason for hiding this comment

RX14 May 26, 2020

Choose a reason for hiding this comment

bcardiff Apr 2, 2020

Choose a reason for hiding this comment

RX14 Apr 2, 2020 • edited

Choose a reason for hiding this comment

waj commented May 20, 2020

asterite commented May 20, 2020

waj commented May 20, 2020

jhass commented May 20, 2020

RX14 commented May 20, 2020 • edited

waj commented May 20, 2020

RX14 commented May 22, 2020 • edited

bcardiff commented May 26, 2020

RX14 commented May 26, 2020

bcardiff left a comment

Choose a reason for hiding this comment

RX14 commented May 26, 2020

bcardiff commented May 26, 2020

RX14 commented May 26, 2020 • edited

RX14 Apr 2, 2020 •

edited

RX14 commented May 20, 2020 •

edited

RX14 commented May 22, 2020 •

edited

RX14 commented May 26, 2020 •

edited